-
Notifications
You must be signed in to change notification settings - Fork 219
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make Client object globally available #1043
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪💚 Flaky test reportTests succeeded. Expand to view the summary
Test stats 🧪
|
Interesting idea! So far, there is nothing in the agent that would make it impossible to have multiple agent instances running, so we'll have to make sure we don't break any relevant use cases with this. I think this change would also allow us to tie instrumentation to the agent instance, making it possible to make the list of instrumentations configurable, which would be nice. Not sure how that would interact with the wrapper script that we plan to introduce, though. Semi-related, looks like we forgot another 6.0 breaking change :( : apm-agent-python/elasticapm/handlers/logging.py Lines 60 to 68 in c1a9126
|
Do you have any use cases in mind? In the "multiple agent instances" case we end up with separate client instances anyway before this PR, right?
Luckily we get to write the wrapper script so I don't expect it to be a problem. :) |
I realized that I don't really need to change every instance of I can't think of any other places that will be hit adversely by this change, so I think this is ready for review. |
def test_get_client(django_elasticapm_client): | ||
with mock.patch.dict("os.environ", {"ELASTIC_APM_METRICS_INTERVAL": "0ms"}): | ||
client2 = get_client("elasticapm.base.Client") | ||
try: | ||
assert get_client() is get_client() | ||
assert client2.__class__ == Client | ||
finally: | ||
get_client().close() | ||
client2.close() | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Django's get_client()
is only ever called with an argument here, in this test. Thus, I don't think we need the type-checking functionality that was in the old implementation. I just removed the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of simplifications,I like!
It looks like there's a use case that this would break. I create an issue for it to investigate a possible bug. |
* Make Client object globally available * Remove global client object on Client.close() * Fix django's get_client to use elasticapm.get_client * Clean up more client creation * CHANGELOG
What does this pull request do?
Managing the Client object has always been a bit of a pain. Most of the time passing around the Client via the framework's application objects works ok. But as we move into supporting more instrumentations that require creating transactions (like Kafka), getting the client object becomes more of a pain.
The idea behind this (unfinished) PR is to painlessly save the Client object when it is created, and provide
elasticapm.get_client()
to make it easy to get that Client object from anywhere.If we decide this is a good idea, the next step is to remove all of the framework-specific scaffolding around the Client object and use this new
elasticapm.get_client()
everywhere.@beniwohli thoughts?